Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix opaque types leaking rhs when inlined and found in type params (and a related stale symbol issue) #22655

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jchyb
Copy link
Contributor

@jchyb jchyb commented Feb 24, 2025

This PR fixes the 2 issues found in #20449, split into 2 commits.

The first commit fixes the stale symbol related issue found if the files from the issue minimization are compiled together. After suspending and retrying compilation, the classDefs that are defined directly in packages previously would sometimes not have companion objects regenerated, instead relying on the stale symbols from the previous run, causing them not to to pass the reallyExists check when looking for a specific ref. Now we make sure to go through lastKnwonDenotation, since the current one may not exists and may not point us to a Module flag when checking if to regenerate it.

The second commit fixes the opaque type alias rhs leaking in a macro. That was caused by building proxies for all parts of the type, including type arguments to opaque types - from the perspective of a type like Object[OpaqueType], the opaque type rhs should not be visible.

@jchyb jchyb force-pushed the fix-i20449-stale-opaque branch from a1755a6 to 4d937b2 Compare February 27, 2025 14:38
@jchyb jchyb requested a review from dwijnand February 28, 2025 09:31
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM, couple of requests

@@ -399,7 +399,9 @@ class Inliner(val call: tpd.Tree)(using Context):
* type aliases, add proxy definitions to `opaqueProxies` that expose these aliases.
*/
private def addOpaqueProxies(tp: Type, span: Span, forThisProxy: Boolean)(using Context): Unit =
tp.foreachPart {
val foreachTpPart =
(p: Type => Unit) => if (forThisProxy) tp.foreachPartWithoutTypeParams(p) else tp.foreachPart(p)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't introduce foreachPartWithoutTypeParams for one use case. Also, it's a confusing name, because you're skipping applied type arguments, rather than "type parameters".

@@ -712,7 +712,7 @@ class Namer { typer: Typer =>
enterSymbol(classConstructorCompanion(classSym.asClass))
else
for moduleSym <- companionVals do
if moduleSym.is(Module) && !moduleSym.isDefinedInCurrentRun then
if moduleSym.lastKnownDenotation.is(Module) && !moduleSym.isDefinedInCurrentRun then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a little detail in a comment about this usage (with any references that would be helpful)

@jchyb jchyb force-pushed the fix-i20449-stale-opaque branch from 4d937b2 to b3bb4ef Compare March 3, 2025 13:17
@jchyb jchyb force-pushed the fix-i20449-stale-opaque branch from b3bb4ef to 23911df Compare March 3, 2025 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants